Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QA] Move integration tests to vizro #950

Merged
merged 35 commits into from
Feb 4, 2025
Merged

Conversation

l0uden
Copy link
Contributor

@l0uden l0uden commented Jan 13, 2025

Description

Here's the first iteration of tests moving from vizro-qa to vizro repo.
Here I moved (and refactored) part of the default dashboard, test helpers and tests for filters, pages and themes. Also created CI for running the tests
The aim of this PR is to find the way of organising this tests in the proper way, so I can move the rest of them after in the same manner

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

Copy link
Contributor

github-actions bot commented Jan 13, 2025

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2025-02-04 14:28:51 UTC
Commit: 7ebae43

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/

@l0uden l0uden requested a review from petar-qb January 14, 2025 10:02
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me so far! 🚀 Amazing work, @l0uden! 🥇

That said, I think we should discuss a few key topics on a call rather than communicating everything through PR comments. Moving all the tests from the vizro-qa repo to the vizro repo is a significant change and potentially introduces some risks (like unoptimised folder structure...). It’s also a great opportunity to optimise while changes are still manageable and cheap because when the full migration is complete, making similar updates could become much more complex.

What do you think about this @antonymilne @Joseph-Perkins?
To reduce risks and streamline the process, I propose setting up the call about the Vizro tests architecture. Addressing these points now will save time and effort later, and it will help us to merge this PR faster (than communicating these things over the PR comments), allowing @l0uden to proceed with the test migration smoothly.

Some topics/questions I'd like to discuss:

  1. Should we add a dedicated test hatch environment?
  2. Can we improve the folder structure for better maintainability?
  3. Should we combine vizro-core/tests/integrations with vizro-core/tests/e2e (and apply the same approach to vizro-ai)?

@l0uden
Copy link
Contributor Author

l0uden commented Jan 17, 2025

This looks great to me so far! 🚀 Amazing work, @l0uden! 🥇

That said, I think we should discuss a few key topics on a call rather than communicating everything through PR comments. Moving all the tests from the vizro-qa repo to the vizro repo is a significant change and potentially introduces some risks (like unoptimised folder structure...). It’s also a great opportunity to optimise while changes are still manageable and cheap because when the full migration is complete, making similar updates could become much more complex.

What do you think about this @antonymilne @Joseph-Perkins? To reduce risks and streamline the process, I propose setting up the call about the Vizro tests architecture. Addressing these points now will save time and effort later, and it will help us to merge this PR faster (than communicating these things over the PR comments), allowing @l0uden to proceed with the test migration smoothly.

Some topics/questions I'd like to discuss:

  1. Should we add a dedicated test hatch environment?
  2. Can we improve the folder structure for better maintainability?
  3. Should we combine vizro-core/tests/integrations with vizro-core/tests/e2e (and apply the same approach to vizro-ai)?

Thanks @petar-qb !

Completely agree with you about the call about this, let's discuss time on Monday, when @antonymilne will be available. And also it would be better if he could have time to take a look on what's done.

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐ Looks good to me 🚀

I have some suggestions, please only implement them if they seem useful, and they make sense!

@petar-qb
Copy link
Contributor

Considering all that @maxschulz-COL suggested and everything we aligned on during last week’s call, I’ve prepared the following folder structure proposal. This structure closely resembles what we currently have but introduces the following changes:

  1. Renames e2e/vizro/dashboards/ -> e2e/vizro/component_utils/
  2. Moves tests_utils/e2e/custom_feature_helpers/ under e2e/vizro/custom_components/
  3. Extracts e2e/vizro/dashboards/default/dashboard.py to a higher-level directory: e2e/vizro/dashboards/dashboard.py
  4. Breaks e2e/vizro/dashboards/default/dashboard_pages.py into multiple files and renames the parent folder from default/ to pages/
  5. Defines a new structure of e2e/vizro/test_screenshots/
  6. Move all 6 files from tests_utils/e2e/ into the new e2e/vizro/utils/
  7. Renames helpers.py -> waiters.py
e2e/
├── component_library/
│   └── test_component_library.py
├── vizro/
│   ├── components_utils/  (1. Renames `e2e/vizro/dashboards/` -> `e2e/vizro/component_utils/`)
│   │   ├── assets/
│   │   │   └── ...
│   ├── custom_components/  (2. Moves `tests_utils/e2e/custom_feature_helpers/` under `e2e/vizro/custom_components/`)
│   │   ├── custom_actions/
│   │   │   └── ...
│   │   ├── custom_charts/
│   │   │   └── ...
│   │   ├── custom_components/
│   │   │   └── ...
│   ├── dashboard.py  (3. Extracts `e2e/vizro/dashboards/default/dashboard.py` to a higher-level directory: `e2e/vizro/dashboards/dashboard.py`)
│   ├── pages/  (4. Breaks `e2e/vizro/dashboards/default/dashboard_pages.py` into multiple files and renames the parent folder from `default/` to `pages/`)
│   │   ├── filter_page/
│   │   ├── parameter_page.py
│   │   └── ...
│   ├── test_dom_elements/
│   │   ├── conftest.py
│   │   ├── test_charts.py
│   │   ├── test_filters.py
│   │   ├── test_pages.py
│   │   └── ...
│   ├── test_screenshots/  (5. Defines a new structure of `e2e/vizro/test_screenshots/`)
│   │   ├── screenshot_images/
│   │   │   └── ... 
│   │   └── test_screenshots.py
│   ├── utils/  (6. Move all 6 files from `tests_utils/e2e/` into the new `e2e/vizro/utils/`)
│   │   ├── asserts.py
│   │   ├── checkers.py
│   │   ├── constants.py
│   │   ├── navigation.py
│   │   ├── paths.py
│   │   └── waiters.py  (7. Renames `helpers.py` -> `waiters.py`)
tests_utils/
...

@maxschulz-COL @l0uden @antonymilne what do you think about the seven changes I suggested?

@maxschulz-COL
Copy link
Contributor

Considering all that @maxschulz-COL suggested and everything we aligned on during last week’s call, I’ve prepared the following folder structure proposal. This structure closely resembles what we currently have but introduces the following changes:

  1. Renames e2e/vizro/dashboards/ -> e2e/vizro/component_utils/
  2. Moves tests_utils/e2e/custom_feature_helpers/ under e2e/vizro/custom_components/
  3. Extracts e2e/vizro/dashboards/default/dashboard.py to a higher-level directory: e2e/vizro/dashboards/dashboard.py
  4. Breaks e2e/vizro/dashboards/default/dashboard_pages.py into multiple files and renames the parent folder from default/ to pages/
  5. Defines a new structure of e2e/vizro/test_screenshots/
  6. Move all 6 files from tests_utils/e2e/ into the new e2e/vizro/utils/
  7. Renames helpers.py -> waiters.py
e2e/
├── component_library/
│   └── test_component_library.py
├── vizro/
│   ├── components_utils/  (1. Renames `e2e/vizro/dashboards/` -> `e2e/vizro/component_utils/`)
│   │   ├── assets/
│   │   │   └── ...
│   ├── custom_components/  (2. Moves `tests_utils/e2e/custom_feature_helpers/` under `e2e/vizro/custom_components/`)
│   │   ├── custom_actions/
│   │   │   └── ...
│   │   ├── custom_charts/
│   │   │   └── ...
│   │   ├── custom_components/
│   │   │   └── ...
│   ├── dashboard.py  (3. Extracts `e2e/vizro/dashboards/default/dashboard.py` to a higher-level directory: `e2e/vizro/dashboards/dashboard.py`)
│   ├── pages/  (4. Breaks `e2e/vizro/dashboards/default/dashboard_pages.py` into multiple files and renames the parent folder from `default/` to `pages/`)
│   │   ├── filter_page/
│   │   ├── parameter_page.py
│   │   └── ...
│   ├── test_dom_elements/
│   │   ├── conftest.py
│   │   ├── test_charts.py
│   │   ├── test_filters.py
│   │   ├── test_pages.py
│   │   └── ...
│   ├── test_screenshots/  (5. Defines a new structure of `e2e/vizro/test_screenshots/`)
│   │   ├── screenshot_images/
│   │   │   └── ... 
│   │   └── test_screenshots.py
│   ├── utils/  (6. Move all 6 files from `tests_utils/e2e/` into the new `e2e/vizro/utils/`)
│   │   ├── asserts.py
│   │   ├── checkers.py
│   │   ├── constants.py
│   │   ├── navigation.py
│   │   ├── paths.py
│   │   └── waiters.py  (7. Renames `helpers.py` -> `waiters.py`)
tests_utils/
...

@maxschulz-COL @l0uden @antonymilne what do you think about the seven changes I suggested?

That sounds good to me! Thanks @petar-qb! All perfect except maybe dashboard.py. Could it be dashboard_for_testing.py? (super minor though, feel free to also ignore)

@l0uden
Copy link
Contributor Author

l0uden commented Jan 29, 2025

@maxschulz-COL and @petar-qb, thanks for the amazing comments!

I'll try to summarise here the major changes I made.

  1. Switched all of the moved tests to dash testing methods. That helped to delete a lot of legacy methods and make code more readable.
  2. Added comments to my methods for more understanding.
  3. Split dashboard pages to different files. I left dashboard into the default folder, because there will be also yaml dashboard.
  4. Moved custom_components closer to the dashboard
  5. Created vizro app under tests_utils/e2e with file that used only by vizro tests. asserts will be used both by vizro and component_library tests.

Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good now. 🚀

@l0uden l0uden merged commit 384c5e6 into main Feb 4, 2025
36 checks passed
@l0uden l0uden deleted the qa/move_integration_tests_to_vizro branch February 4, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants